Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't copy an immutable digraph when adding zero vertices #346

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Don't copy an immutable digraph when adding zero vertices #346

merged 1 commit into from
Nov 18, 2020

Conversation

wilfwilson
Copy link
Collaborator

This is similar to #338 and #340, for edges (although I made these changes over a year ago!).

@wilfwilson wilfwilson added minor A label for PRs or issues that are minor in some sense. performance A label for issues or PR related to performance labels Nov 14, 2020
james-d-mitchell
james-d-mitchell requested changes Nov 16, 2020
View reviewed changes
Copy link
Member

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the manual entry needs to be amended to note that the returned graph is not copied if 0 new nodes are added. Since I think this PR changes the behaviour, it's not inconceivable that there's code out there (or in here) that relies on the old behaviour.

@wilfwilson
Copy link
Collaborator Author

I have updated the manual entry for DigraphAddVertices accordingly.

@wilfwilson wilfwilson changed the title Don't copy and immutable digraph when adding zero vertices Don't copy an immutable digraph when adding zero vertices Nov 17, 2020
james-d-mitchell
james-d-mitchell previously approved these changes Nov 18, 2020
View reviewed changes
doc/oper.xml Outdated Show resolved Hide resolved
gap/oper.gi
{D, labels} -> MakeImmutable(DigraphAddVertices(DigraphMutableCopy(D),
labels)));
function(D, labels)
if IsEmpty(labels) then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't name this in your PR, but why is the argument named labels?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this method adds Length(labels) new vertices, where the ith vertex to be added is given the label labels[i].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

@codecov
Copy link

codecov bot commented Nov 18, 2020 •
edited
Loading

Codecov Report

Merging #346 (abf6ba6) into stable-1.3 (5d719e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           stable-1.3     #346   +/-   ##
===========================================
  Coverage       95.25%   95.25%           
===========================================
  Files              49       49           
  Lines           12352    12359    +7     
===========================================
+ Hits            11766    11773    +7     
  Misses            586      586
Impacted Files Coverage Δ
gap/oper.gi 98.82% <100.00%> (+<0.01%) ⬆️

@james-d-mitchell
Copy link
Member

Can you please rebase onto stable-1.3, and then I'll merge @wilfwilson ?

@james-d-mitchell james-d-mitchell merged commit f4b2021 into digraphs:stable-1.3 Nov 18, 2020
@wilfwilson wilfwilson deleted the improve-opers branch November 18, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james-d-mitchell james-d-mitchell james-d-mitchell approved these changes

Assignees
No one assigned
Labels
minor A label for PRs or issues that are minor in some sense. performance A label for issues or PR related to performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wilfwilson @james-d-mitchell